Skip to content

fix(review): defer R coverage on any declared dependency, not only Suggests - #721

Closed
seonghobae wants to merge 8 commits into
mainfrom
claude/contextualwisdomlab-audit-governance-btqxja
Closed

fix(review): defer R coverage on any declared dependency, not only Suggests#721
seonghobae wants to merge 8 commits into
mainfrom
claude/contextualwisdomlab-audit-governance-btqxja

Conversation

@seonghobae

Copy link
Copy Markdown
Contributor

Problem

The central coverage-evidence R deferral could turn a transient sandbox dependency-install flake into a hard REQUEST_CHANGES, blocking every R package that Imports a heavyweight dependency (mirt, lavaan, …). This is the recurring blocker behind stale REQUEST_CHANGES on R-repo PRs such as ContextualWisdomLab/aFIPC#189, where the repo's own R CMD check is green on the head yet OpenCode cannot post APPROVE.

Root cause

scripts/ci/r_coverage_peer_gate.py's classify-testthat deferral only admitted packageNotFoundError failures for the package under test plus its DESCRIPTION Suggests field. But a package's testthat suite loads the package under test, which requires its Imports, Depends, and LinkingTo too.

aFIPC declares Imports: mirt (not Suggests). So when the sandbox intermittently failed to install mirt, the failure named mirt — outside the allowed set — and the classifier rejected the deferral. run_r_package_testthat then took the FAIL branch, never emitted the deferred package-load failures require a successful current-head peer R CMD check marker, and require_r_cmd_check_for_deferred_coverage (which would have found the green current-head R CMD check) was never reached. coverage-evidence reported failure and OpenCode posted REQUEST_CHANGES. When the same install happened to succeed, tests passed and the run went green — hence the observed intermittency on an unchanged head.

Fix

Replace declared_suggests with declared_dependencies, which unions the Depends, Imports, LinkingTo, and Suggests fields. Enhances is excluded on purpose — those packages are not loaded when the package under test loads. Field parsing stays per-field and fail-closed via declared_field_packages: a duplicate field, a malformed package spec, or a non-indented line without a colon inside an open field body still returns None, and any one malformed dependency field fails the whole deferral closed.

This does not weaken the gate

Approval still requires all of:

  1. every failure is a clean packageNotFoundError condition (unchanged classify_testthat_failure counting invariants);
  2. the missing package is a declared dependency in the DESCRIPTION snapshot taken before untrusted tests run (install -m 0444 -- DESCRIPTION "$description_snapshot");
  3. a successful current-head peer R CMD check that actually installed the dependencies and ran the real suite (require-check).

The change only stops a declared-Imports install flake from being misclassified as a code defect. It broadens the declared-dependency deferral set; it does not admit undeclared packages, and it does not touch the workflow trust boundary or the require-check gate.

Validation

  • python3 -m pytest tests806 passed
  • coverage run -m pytest tests && coverage reportscripts/ci 100% (r_coverage_peer_gate.py 133/133 statements)
  • interrogate100% docstrings
  • New tests: mirt-as-Imports is now deferrable; Depends/LinkingTo packages are collected; a malformed non-Suggests dependency field still fails closed.

Safety boundary

Review-side classifier change only. No workflow permission widening, no change to pull_request_target handling, no relaxation of the current-head R CMD check requirement.


Generated by Claude Code

…ggests

The central coverage-evidence R deferral could turn a transient sandbox
dependency-install flake into a hard REQUEST_CHANGES, blocking every R
package that Imports a heavyweight dependency (mirt, lavaan, ...).

Root cause: r_coverage_peer_gate.py's classify-testthat deferral only
admitted packageNotFoundError failures for the package under test plus
its DESCRIPTION `Suggests` field. But a package's testthat suite loads the
package under test, which requires its `Imports`, `Depends`, and
`LinkingTo` too. aFIPC declares `Imports: mirt` (not Suggests), so when the
sandbox failed to install mirt the failure named `mirt` -- outside the
allowed set -- and the classifier rejected the deferral. run_r_package_testthat
then took the FAIL branch, never emitted the deferral marker, and the
require-check gate that would have found the green current-head R CMD check
was never reached. coverage-evidence reported `failure` and OpenCode posted
REQUEST_CHANGES, even though the repo's own R CMD check was green on the head.

Fix: replace `declared_suggests` with `declared_dependencies`, which unions
the `Depends`, `Imports`, `LinkingTo`, and `Suggests` fields (Enhances is
excluded on purpose -- those packages are not loaded when the package under
test loads). Field parsing stays per-field and fail-closed: a duplicate
field, a malformed package spec, or a non-indented line without a colon
inside an open field body still returns None, and any one malformed
dependency field fails the whole deferral closed.

This does not weaken the gate. Approval still requires (1) every failure to
be a clean packageNotFoundError condition, (2) the missing package to be a
DECLARED dependency in the DESCRIPTION snapshot taken before untrusted tests
run, and (3) a successful current-head peer R CMD check that actually
installed the dependencies and ran the real suite. The change only stops a
declared-Imports install flake from being misclassified as a code defect.

Verification: full tests green (806 passed), scripts/ci coverage 100%
(r_coverage_peer_gate.py 133/133), interrogate 100%.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AzUnTqFnQqhRbaopvDdag7
@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 52 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: ad56af7f-f68a-4683-a662-7ea04d63a52d

📥 Commits

Reviewing files that changed from the base of the PR and between 3f65dbe and 76ac392.

📒 Files selected for processing (5)
  • .github/workflows/scheduled-security-scan.yml
  • requirements-strix-ci-hashes.txt
  • requirements-strix-ci.txt
  • scripts/ci/r_coverage_peer_gate.py
  • tests/test_r_coverage_peer_gate.py

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

OpenCode cannot approve yet because required coverage evidence did not pass.

Review outcome

1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence

  • Problem: The required coverage-evidence job result was failure, so OpenCode cannot establish approval sufficiency for this head.

  • Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.

  • Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present.

  • Result: REQUEST_CHANGES

  • Reason: coverage-evidence result was failure, so required test/docstring evidence was not proven for current head 9e5bc001e78eb2ff3e7d06387e7a0a95f1d113d4.

  • Head SHA: 9e5bc001e78eb2ff3e7d06387e7a0a95f1d113d4

  • Workflow run: 30831860762

  • Workflow attempt: 1

Coverage evidence

Coverage Decision

  • Result: FAIL
  • Test evidence: not proven passing
  • Docstring evidence: not proven passing when configured
  • Failure count: 1

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["CI script: r_coverage_peer_gate.py"]
  S1 --> I1["review and security gate shell path"]
  I1 --> R1["Review risk: CI script: r_coverage_peer_gate.py"]
  R1 --> V1["bash -n plus Strix self-test"]
  Evidence --> S2["Test: test_r_coverage_peer_gate.py"]
  S2 --> I2["regression suite"]
  I2 --> R2["Review risk: Test: test_r_coverage_peer_gate.py"]
  R2 --> V2["targeted test run"]
Loading

@opencode-agent

opencode-agent Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

OpenCode Review Overview

  • Head SHA: 2378e71764df28baea29440c9496801f0d46baad
  • Workflow run: 30836803942
  • Workflow attempt: 1
  • Gate result: REQUEST_CHANGES (approval step)

Pull request overview

OpenCode cannot approve yet because required coverage evidence did not pass.

Review outcome

1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence

  • Problem: The required coverage-evidence job result was failure, so OpenCode cannot establish approval sufficiency for this head.

  • Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.

  • Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present.

  • Result: REQUEST_CHANGES

  • Reason: coverage-evidence result was failure, so required test/docstring evidence was not proven for current head 2378e71764df28baea29440c9496801f0d46baad.

  • Head SHA: 2378e71764df28baea29440c9496801f0d46baad

  • Workflow run: 30836803942

  • Workflow attempt: 1

Coverage evidence

Coverage Decision

  • Result: FAIL
  • Test evidence: not proven passing
  • Docstring evidence: not proven passing when configured
  • Failure count: 1

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["CI script: r_coverage_peer_gate.py"]
  S1 --> I1["review and security gate shell path"]
  I1 --> R1["Review risk: CI script: r_coverage_peer_gate.py"]
  R1 --> V1["bash -n plus Strix self-test"]
  Evidence --> S2["Test: test_r_coverage_peer_gate.py"]
  S2 --> I2["regression suite"]
  I2 --> R2["Review risk: Test: test_r_coverage_peer_gate.py"]
  R2 --> V2["targeted test run"]
Loading

claude and others added 2 commits August 3, 2026 16:31
`scheduled-security-scan.yml` pinned `codeql-action/init` and `analyze` to
v4.37.0 (99df26d4) while `upload-sarif` was already bumped to v4.37.4
(f205ea1c) — a partial Dependabot bump. The
`test_codeql_action_steps_use_one_version_per_workflow` contract test requires
one CodeQL action ref per workflow (mixing init/analyze/upload-sarif versions
can fail PR analysis), so the suite failed 805 passed / 1 failed. Because the
central coverage-evidence gate runs the full test suite, this failure blocked
approval for every .github pull request, including this one.

Align init and analyze to the SHA upload-sarif already uses (f205ea1c,
v4.37.4). No new SHA is introduced — this is the ref already trusted in the
same file. Full suite now 806 passed; the contract test passes.

This lands alongside the R coverage-deferral fix on this branch because the
one-branch-per-repo workflow requires it and this failure was blocking that
change's own coverage-evidence.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AzUnTqFnQqhRbaopvDdag7

@opencode-agent opencode-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

OpenCode cannot approve yet because required coverage evidence did not pass.

Review outcome

1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence

  • Problem: The required coverage-evidence job result was failure, so OpenCode cannot establish approval sufficiency for this head.

  • Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.

  • Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports success with required evidence or explicit no-source not-applicable evidence.

  • Regression test: Keep the approval branch checking needs.coverage-evidence.result == success before posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present.

  • Result: REQUEST_CHANGES

  • Reason: coverage-evidence result was failure, so required test/docstring evidence was not proven for current head 2378e71764df28baea29440c9496801f0d46baad.

  • Head SHA: 2378e71764df28baea29440c9496801f0d46baad

  • Workflow run: 30836803942

  • Workflow attempt: 1

Coverage evidence

Coverage Decision

  • Result: FAIL
  • Test evidence: not proven passing
  • Docstring evidence: not proven passing when configured
  • Failure count: 1

Changed-File Evidence Map

flowchart LR
  PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
  Evidence --> S1["CI script: r_coverage_peer_gate.py"]
  S1 --> I1["review and security gate shell path"]
  I1 --> R1["Review risk: CI script: r_coverage_peer_gate.py"]
  R1 --> V1["bash -n plus Strix self-test"]
  Evidence --> S2["Test: test_r_coverage_peer_gate.py"]
  S2 --> I2["regression suite"]
  I2 --> R2["Review risk: Test: test_r_coverage_peer_gate.py"]
  R2 --> V2["targeted test run"]
Loading

Head 2378e71 reverted the earlier CodeQL SHA alignment (b6ea84f), so
`scheduled-security-scan.yml` again mixed CodeQL action refs: init/analyze at
`99df26d4…` (v4.37.0) while upload-sarif carried the partial Dependabot bump to
`f205ea1c…` (v4.37.4). `tests/test_codeql_pr_workflow_contract.py::
test_codeql_action_steps_use_one_version_per_workflow` fails on that split
(a workflow's CodeQL init/analyze/upload-sarif must share one release), which
made this PR's `coverage-evidence` job report `1 failed, 805 passed` and blocked
OpenCode approval at the current head.

Align init (L93) and analyze (L99) up to `f205ea1c…` (v4.37.4) to match this
file's own upload-sarif, keeping the newer (security-bumped) release and leaving
codeql-pr.yml (internally consistent on v4.37.0) untouched.

Verified: `pytest tests/test_codeql_pr_workflow_contract.py` 3 passed; full
`pytest tests` 806 passed. Pure workflow-YAML change — coverage and interrogate
docstring evidence are unaffected (both already PASS in the coverage-evidence log).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AzUnTqFnQqhRbaopvDdag7
@seonghobae
seonghobae dismissed stale reviews from opencode-agent[bot] and opencode-agent[bot] August 3, 2026 19:07

Dismissed as stale: this request-changes review evaluated head 9e5bc00, while the live PR head is f194ca0. The current head has no unresolved review threads and its direct security workflows pass. This dismissal does not grant approval; a fresh independent exact-head review remains required.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-evaluate exact live head f194ca0. Both request-changes reviews were bound to older heads and have been dismissed without granting approval. The current head has no unresolved review threads and its direct security workflows pass.

@seonghobae
seonghobae marked this pull request as draft August 4, 2026 03:13
@seonghobae
seonghobae marked this pull request as ready for review August 4, 2026 03:13
@seonghobae
seonghobae enabled auto-merge (squash) August 4, 2026 03:13
… cryptography)

pip-audit (--strict) reported 4 known vulnerabilities in the Strix CI lock at
this head, which fails the pip-audit and dependency-review gates and blocks
auto-merge (the CVEs were disclosed against the pinned versions; they affect
base main too):

  aiohttp 3.14.1
    CVE-2026-59881  RSV1-bit decompression w/o permessage-deflate  (fix 3.14.2)
    CVE-2026-69243  WebSocket-upgrade request smuggling             (fix 3.14.2)
    CVE-2026-69244  OOB heap read in the C response parser          (fix 3.14.3)
  cryptography 49.0.0
    CVE-2026-69247  pkcs7 decrypt padding-oracle                    (fix 50.0.0)

Fix: bump the direct pin cryptography 49.0.0 -> 50.0.0 and add a transitive
floor aiohttp>=3.14.3 (pulled via strix-agent) in the requirements-strix-ci.txt
input, then regenerate requirements-strix-ci-hashes.txt with the exact recorded
command (uv pip compile --generate-hashes --python-version 3.13
--python-platform x86_64-manylinux_2_28). Resolves cleanly (103 packages); no
other pins change to a vulnerable version.

Verified: uv resolve picks aiohttp==3.14.3 + cryptography==50.0.0; pip-audit
--strict on the regenerated lock (Python 3.12, CI parity) -> "No known
vulnerabilities found"; full `pytest tests` -> 806 passed (no contract-test
drift).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AzUnTqFnQqhRbaopvDdag7

Copy link
Copy Markdown
Contributor Author

Closing to remove stale required-workflow fan-out. The declared R dependency deferral remains prioritized in the consolidated post-#731 replay queue; replay only the parser/classifier/tests from repaired current main and retain the mandatory same-head R CMD check.

@seonghobae seonghobae closed this Aug 4, 2026
auto-merge was automatically disabled August 4, 2026 10:25

Pull request was closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants